Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[docs] Restructure and expand presto_cpp docs #22717

Merged
merged 1 commit into from
May 16, 2024
Merged

[docs] Restructure and expand presto_cpp docs #22717

merged 1 commit into from
May 16, 2024

Conversation

steveburnett
Copy link
Contributor

@steveburnett steveburnett commented May 10, 2024 •
edited
Loading

Description

  • Moved Prestissimo Developer Guide documentation files to "presto_cpp" directory, including updating index files
  • Revised text references from "Prestissimo" to "Presto C++" or "C++ based Presto"
  • Added new content for Motivation, Vision, Supported Use Cases, and significant expansion of existing Limitations content (draft content provided by @tdcmeehan, thanks!)

Motivation and Context

Updates and expands C++ based Presto documentation.

Impact

Documentation.

Test Plan

Test in local build before opening PR, then CI.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... Improve C++ based Presto documentation :pr:`22717`

@steveburnett steveburnett self-assigned this May 10, 2024
@steveburnett steveburnett requested a review from a team as a code owner May 10, 2024 20:04
@github-actions github-actions bot added the docs label May 10, 2024
elharo
elharo reviewed May 11, 2024
View reviewed changes
presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto-cpp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/features.rst Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/limitations.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/limitations.rst Outdated Show resolved Hide resolved
@steveburnett
Copy link
Contributor Author

All comments addressed, commits squashed, ready!

elharo
elharo previously approved these changes May 14, 2024
View reviewed changes
@tdcmeehan tdcmeehan self-assigned this May 14, 2024
majetideepak
majetideepak reviewed May 16, 2024
View reviewed changes
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steveburnett looks great. One comment.

presto-docs/src/main/sphinx/presto-cpp.rst Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/limitations.rst Outdated Show resolved Hide resolved
@steveburnett
Copy link
Contributor Author

Thanks! Review comments addressed, commits squashed, CI running.

@steveburnett steveburnett merged commit fccc180 into prestodb:master May 16, 2024
57 checks passed
@steveburnett steveburnett deleted the steveburnett-presto_cpp branch May 16, 2024 16:10
aditi-pandit
aditi-pandit reviewed May 16, 2024
View reviewed changes
presto-docs/src/main/sphinx/presto_cpp/limitations.rst

* Not all built-in functions are implemented in C++. Attempting to use unimplemented functions results in a query failure. For supported functions, see `Function Coverage <https://facebookincubator.github.io/velox/functions/presto/coverage.html>`_.

* Not all built-in types are implemented in C++. Attempting to use unimplemented types will result in a query failure.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steveburnett : Apologize for the delay. I feel we should add more detail here about the supported and unsupported types here.

The specifics are :
All basic/structured types (from https://prestodb.io/docs/0.286/language/types.html) but [CHAR, TIME, TIME WITH TIMEZONE] are supported. These are subsumed by VARCHAR, TIMESTAMP and TIMESTAMP WITH TIMEZONE.

Caveat : Prestissimo only supports unlimited length Varchar. It does not honor the lengths in varchar[n].

Types IPADDRESS, IPPREFIX, UUID, kHYPERLOGLOG, P4HYPERLOGLOG, QDIGEST, TDIGEST are not supported

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #22772 to add this content to the docs.

@steveburnett steveburnett mentioned this pull request May 16, 2024
Merged
6 tasks
@wanglinsong wanglinsong mentioned this pull request Jun 25, 2024
Merged
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aditi-pandit aditi-pandit aditi-pandit left review comments

@tdcmeehan tdcmeehan tdcmeehan approved these changes

@majetideepak majetideepak majetideepak approved these changes

@elharo elharo elharo left review comments

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@imjalpreet imjalpreet Awaiting requested review from imjalpreet

@agrawalreetika agrawalreetika Awaiting requested review from agrawalreetika

Assignees

@steveburnett steveburnett

@tdcmeehan tdcmeehan

Labels
docs
Projects
Presto Documentation
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@steveburnett @elharo @majetideepak @tdcmeehan @aditi-pandit